Remove RWLock from EntryNotifier because it causes perf degradation #33797

vovkman · 2023-10-20T21:16:20Z

Problem

When upgrading to 1.16 we observed that some of our geyser nodes could not keep up with the chain and had wild CPU fluctuations (running on 7443p). The first solution we found was to increase CPU (7543p), which sort of solved the issue. We profiled a local cluster to find any hotspots and found that the EntryNotifier was using significantly more CPU time than any other Geyser method. It seemed to be due to some thread contention from recv_timeout or the write lock is was obtaining. As a simple first step I removed the RWLock on EntryNotifier because it is unnecessary and re profiled. This removed the CPU overhead. I tested this change on MB with our geyser plugins and they are now stable on 7443p again.

Here is an example running yellowstone grpc plugin. This is from running a local cluster for about 10 minutes and shows he perf improvement. On MB the impact is much more severe, I didn't profile there though due to time/perf data size. Since a test of this change on MB fixed the issue I felt this was enough to illustrate the problem/solution.

With RWLock - 0.234% CPU time

Without RWLock - 0.147% CPU time

Summary of Changes

Remove RWLock from EntryNotifier

…hen entry notifications are enabled on geyser

…emove-rwlock-entry-notifier

lijunwangs · 2023-10-27T19:55:04Z

ledger/src/entry_notifier_interface.rs

@@ -8,4 +8,4 @@ pub trait EntryNotifier {
    fn notify_entry(&self, slot: Slot, index: usize, entry: &EntrySummary);
 }

-pub type EntryNotifierLock = Arc<RwLock<dyn EntryNotifier + Sync + Send>>;


Please remove the unused RwLock to make sure clippy is happy

…emove-rwlock-entry-notifier

vovkman · 2023-11-01T16:06:22Z

@lijunwangs do these remaining buildkite steps need to pass? They seem to be pulling in the wrong dependencies and failing bc of that

CriesofCarrots · 2023-11-01T16:11:24Z

@lijunwangs do these remaining buildkite steps need to pass? They seem to be pulling in the wrong dependencies and failing bc of that

Those failures seem legitimate, and not related to dependencies:

Checking solana-geyser-plugin-manager v1.18.0 (/solana/geyser-plugin-manager)
error[E0277]: the trait bound `std::sync::RwLock<EntryNotifierImpl>: EntryNotifier` is not satisfied
   --> geyser-plugin-manager/src/geyser_plugin_service.rs:105:18
    |
105 |             Some(Arc::new(RwLock::new(entry_notifier)))
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `EntryNotifier` is not implemented for `std::sync::RwLock<EntryNotifierImpl>`
    = help: the trait `EntryNotifier` is implemented for `EntryNotifierImpl`
    = note: required for the cast from `Arc<std::sync::RwLock<EntryNotifierImpl>>` to `Arc<dyn EntryNotifier + std::marker::Send + Sync>`

For more information about this error, try `rustc --explain E0277`.
could not compile `solana-geyser-plugin-manager` (lib) due to previous error

…emove-rwlock-entry-notifier

codecov · 2023-11-01T17:34:16Z

Codecov Report

Merging #33797 (14e38fa) into master (63abc72) will increase coverage by 0.0%.
Report is 1 commits behind head on master.
The diff coverage is 0.0%.

@@           Coverage Diff           @@
##           master   #33797   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         809      809           
  Lines      218253   218252    -1     
=======================================
+ Hits       178771   178821   +50     
+ Misses      39482    39431   -51

vovkman · 2023-11-01T17:35:50Z

@lijunwangs do these remaining buildkite steps need to pass? They seem to be pulling in the wrong dependencies and failing bc of that

Those failures seem legitimate, and not related to dependencies:

Checking solana-geyser-plugin-manager v1.18.0 (/solana/geyser-plugin-manager)
error[E0277]: the trait bound `std::sync::RwLock<EntryNotifierImpl>: EntryNotifier` is not satisfied
   --> geyser-plugin-manager/src/geyser_plugin_service.rs:105:18
    |
105 |             Some(Arc::new(RwLock::new(entry_notifier)))
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `EntryNotifier` is not implemented for `std::sync::RwLock<EntryNotifierImpl>`
    = help: the trait `EntryNotifier` is implemented for `EntryNotifierImpl`
    = note: required for the cast from `Arc<std::sync::RwLock<EntryNotifierImpl>>` to `Arc<dyn EntryNotifier + std::marker::Send + Sync>`

For more information about this error, try `rustc --explain E0277`.
could not compile `solana-geyser-plugin-manager` (lib) due to previous error

Oh rip I must have missed a push. Updated, all passing now

lijunwangs

Looks good

Remove RWLock from EntryNotifier because it causes perf degradation w…

a5e055f

…hen entry notifications are enabled on geyser

mergify bot added community Community contribution need:merge-assist labels Oct 20, 2023

mergify bot requested a review from a team October 20, 2023 21:16

Merge branch 'master' of https://github.com/solana-labs/solana into r…

cba1199

…emove-rwlock-entry-notifier

vovkman changed the title ~~Remove RWLock from EntryNotifier because it causes perf degradation w…~~ Remove RWLock from EntryNotifier because it causes perf degradation Oct 20, 2023

CriesofCarrots added the CI Pull Request is ready to enter CI label Oct 27, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Oct 27, 2023

lijunwangs added the CI Pull Request is ready to enter CI label Oct 27, 2023

solana-grimes removed the CI Pull Request is ready to enter CI label Oct 27, 2023

lijunwangs reviewed Oct 27, 2023

View reviewed changes

vovkman added 2 commits October 27, 2023 13:08

remove unused RWLock

5228b70

Merge branch 'master' of https://github.com/solana-labs/solana into r…

5aec064

…emove-rwlock-entry-notifier

vovkman added 2 commits November 1, 2023 09:15

Remove RWLock

11d7615

Merge branch 'master' of https://github.com/solana-labs/solana into r…

14e38fa

…emove-rwlock-entry-notifier

lijunwangs approved these changes Nov 6, 2023

View reviewed changes

lijunwangs merged commit e840b97 into solana-labs:master Nov 6, 2023
17 checks passed

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove RWLock from EntryNotifier because it causes perf degradation #33797

Remove RWLock from EntryNotifier because it causes perf degradation #33797

vovkman commented Oct 20, 2023 •

edited

Loading

lijunwangs Oct 27, 2023

vovkman Oct 27, 2023

vovkman commented Nov 1, 2023

CriesofCarrots commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 1, 2023

vovkman commented Nov 1, 2023

lijunwangs left a comment

Remove RWLock from EntryNotifier because it causes perf degradation #33797

Remove RWLock from EntryNotifier because it causes perf degradation #33797

Conversation

vovkman commented Oct 20, 2023 • edited Loading

Problem

Summary of Changes

lijunwangs Oct 27, 2023

Choose a reason for hiding this comment

vovkman Oct 27, 2023

Choose a reason for hiding this comment

vovkman commented Nov 1, 2023

CriesofCarrots commented Nov 1, 2023 • edited Loading

codecov bot commented Nov 1, 2023

Codecov Report

vovkman commented Nov 1, 2023

lijunwangs left a comment

Choose a reason for hiding this comment

vovkman commented Oct 20, 2023 •

edited

Loading

CriesofCarrots commented Nov 1, 2023 •

edited

Loading